Codecov Report

Merging #2009 (22d9f5d) into main (ffc7a3e) will increase coverage by 0.09%.
The diff coverage is 64.61%.

@@            Coverage Diff             @@
##             main    #2009      +/-   ##
==========================================
+ Coverage   45.89%   45.98%   +0.09%     
==========================================
  Files         206      207       +1     
  Lines       17150    17213      +63     
==========================================
+ Hits         7871     7916      +45     
- Misses       9279     9297      +18     

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffc7a3e...22d9f5d. Read the comment docs.

Hi @lupd,

Thanks for the contribution!
I did a quick review of the code and the implementation looks mostly very good, but I think there is one critical part missing; the Weak in WeakSet ;)

Objects that are added to a WeakSet should only be added as a weak reference. That means that the garbage collector can in theory collect the object references, if they are not strongly referenced elsewhere.

Because it is tricky (impossible?) to demonstrate this with WeakSet, here is some code that shows this behaviour with a WeakRef. The principle is the same:

let ref;

{
    let obj = {};
    ref = new WeakRef(obj);

    // This will always be `{}`
    ref.deref()
}

// `obj` is now out of scope.
// This means that the object that was assigned to `obj` is now only weakly referenced by `ref`.
// The gc can now collect the object.

// This may be `{}` if the object was not yet collected.
// But at some point it will be `undefined` because the weak reference has been collected.
ref.deref()

If you want to try that code in node, I recommend starting node with --expose-gc and using global.gc() a bunch of times before the last ref.deref(). It does not seem completely deterministic, so you may also want to try running node a bunch of times. But you will definitely see the behavior sometimes.

I think this behavior is not possible with our current gc in boa. We are using rust-gc as our garbage collector and that crate does not implement weak gc references as far as I know.

We definitely weed weak gc references in boa for WeakRef, WeakSet and WeakMap. If you want to take a stab at this @lupd, the best place to start would probably be this issue in rust-gc, where weak references are being discussed: Manishearth/rust-gc#65. If it seems reasonable it would be nice to get weak references contributed to rust-gc, otherwise we would probably have to implement our own gc.

@lupd did you have the chance to look into the comment by @raskad ? Can we help you with this?

I think this is blocked on having weak references in the garbage collector.

I think this is blocked on having weak references in the garbage collector.

True :) There seems to be a draft PR for this: Manishearth/rust-gc#148

It's a bit of a work in progress...on a couple fronts 😅

@lupd Some weak types for the Gc were added with #2394 merged, so I think this should be workable 😄